feat(omnigraph): add Omnigraph target connector - #2371
Draft
pronskiy wants to merge 40 commits into
Draft
Conversation
Node and edge types with additive .pg schema evolution, synthetic coco_key addressing, scratch-branch commits for oversized syncs, and reactive endpoint stubs for edges that race ahead of their nodes. Ships with docs and a meeting-notes knowledge-graph example.
A failed scratch-branch deletion was silently suppressed even after the branch had been created and merged. That made an update appear successful while leaving a branch that prevents later schema changes.\n\nTrack successful branch creation, skip cleanup only when creation itself failed, and otherwise propagate deletion errors so operators can recover before CocoIndex commits tracking state.
Omnigraph accepts only a complete schema document, so concurrent read-merge-write cycles could both read the same revision and then replace one another's changes. This could silently remove a type and its data.\n\nHold a store-scoped advisory file lock across the complete schema transaction. Concurrent connector instances on the host now observe the preceding write before merging their own schema changes.
The new connector failed the repository's required Ruff checks because it carried an unused protocol and imports, invalid generic base ordering, implicit test string concatenation, and subprocess and datetime calls that violated enabled rules. Remove dead code, organize and format the modules, and make the test intent explicit so the pre-commit lint stage can accept the branch.
The connector imported the Unix-only fcntl module during test collection and kept named temporary files open while the CLI reopened them. This broke the supported Windows test matrix and runtime before any Omnigraph operation could complete. Use platform-specific file locking and close temporary payload files before invoking the CLI, while retaining deterministic cleanup afterward.
Explicit PropertyDef schemas bypassed the validation used by inferred OmnigraphType annotations. A crafted or malformed pg_type could therefore break out of a property declaration and alter the whole schema fragment applied to the graph. Validate every property type at the shared renderer so inferred and manually constructed schemas enforce the same grammar.
Endpoint retry stubs inferred their parameter type from the runtime Python value. That discarded OmnigraphType overrides such as I32 or U64, so an otherwise valid edge could fail only when its endpoint had not been written yet. Carry each endpoint's full key property definition into the edge action and reuse the normal encoder when building recovery stubs.
Omnigraph refuses schema changes while any non-main branch exists. Entity batches created scratch branches outside the schema lock, so a concurrent type update could observe the temporary branch and fail even though cleanup completed normally. Use one store-scoped lock for schema reconciliation and the complete scratch-branch lifecycle, and document the remaining cross-host coordination boundary.
A USER-to-SYSTEM transition filtered the previous user-managed record out of the write diff. When ownership and schema changed together, the connector emitted no DDL but still persisted the new SYSTEM tracking record, leaving the graph schema stale. Diff SYSTEM declarations against every previously tracked schema so the ownership handoff applies any accompanying schema change while USER declarations remain read-only.
The missing-endpoint parser treated the first apostrophe as the end of a string key. Valid identifiers such as O'Brien therefore bypassed endpoint-stub recovery and caused an otherwise recoverable edge mutation to fail. Match the closing quote through the complete CLI error suffix and cover embedded apostrophes and empty keys with regression tests.
The new Omnigraph meeting-notes example split third-party imports into two groups and placed cocoindex after unrelated packages. Ruff reports this as I001, so the branch failed the repository's documented lint check. Keep the imports in Ruff's canonical order so the example passes the same checks as connector code.
The endpoint retry loop allowed two stub rounds per commit, on the reasoning that an edge has two endpoints. But a commit carries every edge of a component, and the engine names only the first missing endpoint per attempt, so any commit whose edges referenced three or more absent nodes failed on its third attempt with the engine's "not found" error. Two edges with four missing endpoints reproduced it; a shared source with two new targets was already enough. Bound the loop by the distinct endpoints the batch's edges reference instead: each (type, key) is stubbed at most once, and a stub is only built for an endpoint some edge in the batch names, so a "not found" that repeats or names a foreign endpoint still propagates.
Change detection fingerprinted the raw Python property values and only then applied `PropertyDef.encoder`. Swapping an encoder therefore changed every value the graph should hold while every fingerprint stayed the same, so no write was scheduled at all. Encode first and fingerprint the result, which is the record the graph actually holds. Encoding was also incomplete: `list[datetime.date]` mapped to `[Date]`, which the schema accepts, but only the bare `Date`/`DateTime` scalars had an encoder, so every write of such a node failed in `json.dumps`. A list type now gets its element's encoder mapped over the elements. While here, check an edge endpoint id against the endpoint type's own key at `declare_edge`: the id is rendered with `str()`, so an integer given for a String-keyed endpoint addressed whichever node's key happened to be those digits, and a string given for an integer-keyed endpoint could never match anything.
`_CliClient._run` awaited `proc.communicate()` with nothing around it. asyncio does not touch the child when the awaiting task is cancelled, so cancelling an update mid-call left the omnigraph process running to completion on its own: a cancelled `mutate` still landed its commit, a cancelled `schema apply` still rewrote the schema, and the connector had already moved on. Verified with a probe: the child was alive well after the cancellation and only died at interpreter exit. On any interruption, kill the child, wait for it so it is reaped, and then let the exception propagate.
A multi-commit sync runs on a `coco_scratch_<uuid>` branch that a `finally` deletes. That covers exceptions, but a process killed between creating and deleting the branch left it behind, and Omnigraph refuses every `schema apply` while any non-main branch exists. Nothing ever listed or deleted such a branch, so one killed process blocked every later schema change on that store for good. Reproduced: a manually created `coco_scratch_deadbeef` failed two consecutive updates. Recover where it bites: when `schema apply` is refused for non-main branches, list the branches, delete those under the connector's own prefix, and retry once. This runs under the store lock, which a live scratch branch is held in for its whole lifetime, so any scratch branch a lock holder can see is provably abandoned. User branches are never touched; a refusal caused by one propagates unchanged. Also fold branch creation, the lock, and cleanup into one `_scratch_branch` context manager so the lifecycle has a single owner.
A `managed_by=user` type whose declaration differed from the one the connector had tracked was rejected with advice to apply the migration with `omnigraph schema apply`. Reconcile only ever compares tracking records in LMDB and never reads the live schema, so applying that migration changed nothing: the next run raised the same error, and so did every run after it. The only ways out were switching the type to system management or dropping the app. User-managed means the schema is the user's. Track the declaration as it is and write no schema; a write that names a property the graph lacks fails with the engine's own error, which names the property. The non-nullable-addition guard stays, but only for system-managed types, since it is about a schema write this connector would itself issue.
`schema show` returns the schema source verbatim, formatting and
comments included, so a schema somebody wrote or edited by hand is what
the merger has to edit. The merger recognised `node X {` only at the
start of a line, ended a block at the first `}` it saw, and appended
whatever it failed to find. Verified against the engine: an indented
block or two declarations on one line made it append a duplicate
("duplicate node name in schema shape"), and a `//` comment containing
`}` made it cut the block short and leave a stray `} here` behind
("expected EOI or schema_decl"). Each produced a schema the engine
refused, on the whole-graph write path where a bad edit costs every
type.
Replace the line-anchored regexes with a scanner: blank `//` comments to
spaces of equal length so offsets hold, find `node NAME` / `edge NAME`
heads anywhere, take a braced body to its matching `}` and a brace-less
edge to the end of its endpoints, and resume after each block so a
property named `node` or `edge` is never taken for a declaration.
Dropping an app with two node types and a connecting edge type failed and left a node type behind. Every mounted type is its own processing component, so the three removals reach the schema sink as separate batches, and whichever node type's batch ran before the edge type's hit the orphaned-endpoint guard: Omnigraph rejects a schema whose edge points at a missing node. A second drop then finished the job. Waiting in the sink for the edge's removal cannot work: all type removals share one sink batcher, so the edge type's batch is queued behind the very batch that would wait for it (verified live — the wait ran to its timeout). Instead, a node type's drop removes the connector's own referencing edge types in the same schema write; the edge type's batch then finds nothing to remove and writes nothing. Ownership has to be explicit for that to be safe: a `managed_by=user` type must declare `coco_key` too, so the property alone cannot tell the connector's types from a user's. Every block the connector renders now carries a `// managed by cocoindex` comment on its `coco_key` line, which `schema show` preserves. An edge type without it is never removed on the user's behalf; the drop fails naming it. A `@key` change on a referenced node type still fails, since the edge stays declared. Types created before this change lack the marker until their block is next rendered; the branch is unreleased, so no migration is provided.
`Mutation` carried four overlapping views of one statement — the
rendered `expr`, the `params`, the `binds` the signature was built
from, and the unwrapped `body` — and `combine_mutations` merged several
of them by regex-renaming every `$name` in each body to `s{i}_name`
before rendering again. Every builder produced a standalone rendering
that only tests ever looked at, since the sink always went through the
combiner.
Builders now return a `Statement`: the statement text with a `$?` slot
per bound value, plus the binds (label, type, value) in slot order.
Parameter names are allocated once, in `render_query`, which produces
the `Mutation` the transport sends — one query, one commit — with no
renaming and no regex. `Mutation` is just `expr` and `params` again.
The public surface carried two names for everything: node/edge, plus table/relation/record aliases (`TableTarget`, `mount_relation_target`, `declare_record(row=...)`, ...) that were the same objects under names borrowed from the sibling connectors. `mount_node_target` also took a `key=` that could only repeat the schema's key or raise. And an edge type's properties had to be built with `NodeSchema.from_class`, which requires a `key=` that means nothing for an edge — its identity is always `(from_id, to_id)` — so the example had to explain the dummy key away in a docstring. Keep the graph-native names only, drop the mount-time `key=`, and add `EdgeSchema` (properties, no key) with its own `from_class`. An edge type refuses a `NodeSchema` at mount, where the mistake is made. The example, its README, and the docs use the narrowed surface.
When an edge is written before the component that owns one of its endpoints, the connector inserts a key-only stub for that node. The stub has no owner: it is not a target state, so removing the edge later leaves it behind (reproduced: dropping the only edge left both stub nodes in the graph). That matches the Neo4j connector, whose MERGE creates endpoint nodes it never removes, and tracking stubs would need node ownership shared across components, which the engine does not give a connector. Document it instead, with the way out: declare every endpoint node somewhere.
The connector's acceptance tests — app teardown, missing endpoints, interrupted updates, external migrations, encoded-value changes — drive the real `omnigraph` CLI, and only ran on a machine with a git-ignored local binary and OMNIGRAPH_TEST_STORE=1 set by hand. CI never ran them. Install the pinned 0.10.0 release into test/bin on the Linux and macOS runners it ships for, verified against the published sha256, and set the flag for the pytest hook. Windows and Intel macOS have no release asset, so the live tests keep skipping there.
The paragraph under the user-managed caution still said the connector checks a declaration against what it previously tracked and raises when they differ. That guard was removed two commits ago; say what happens now instead — the declaration is adopted as-is and drift shows up as the engine's own error on the first write that names a missing property.
Create node types and an edge type system-managed, re-declare the edge type with `managed_by="user"`, then drop the app: the drop succeeded and deleted the user's edge type with its edges. The edge block still carried the ownership marker from when the connector rendered it, a user-managed declaration writes no schema, and the node drops read the stale marker as current ownership and took the edge type along. The marker also could not have been removed: the engine stores an applied schema's source only when the apply changes something structural, so a comment-only apply reports `applied: false` and keeps the old source (verified against the binary). Ownership is therefore now a declared property — `coco_managed: Bool?`, nullable and never written — on every block the connector renders, and the handoff is the one schema write a user-managed declaration causes: when a type's previous tracking record says system-managed, reconcile emits a release action and the sink drops that property from the block and nothing else. From then on the block is the user's, and a drop of a node type it references refuses, naming it, instead of removing it.
The store lock was keyed by the raw URI text, so `file:///a/store` and `file:///a/./store` took different locks. A schema apply through one spelling hit the engine's non-main-branch refusal, reaped the scratch branch a process was still using through the other spelling, and succeeded; that process then failed its own cleanup. Reproduced live. Two changes. The lock is now keyed by a canonical store identity: a `file://` URI resolves to its real path, and any other scheme gets a lower-cased scheme and host and a normalised path. And abandonment is established before deletion rather than inferred from visibility: a live scratch branch holds a per-branch liveness lock from before it is created until after it is deleted, and the reaper deletes only a branch whose lock it can take without waiting.
A `NodeSchema` may give any property an encoder, the key included. CocoIndex tracks a node by the raw key value (its target-state key and `coco_key` both derive from it) while the graph keys on the encoded one. Changing the key's encoder from `str.lower` to `str.upper` was therefore detected as a change and upserted — creating a second node "MIXED" next to the existing "mixed", both sharing one `coco_key`, with nothing ever removing the old identity. Reproduced live. The key is the node's identity and has to be declared already normalized, so a custom encoder on a key property is refused at `node_target` with the reason. The built-in `Date`/`DateTime` encoders stay allowed there: they are pure functions of the value and never change, so the raw and encoded identities cannot drift apart.
`is_connector_managed` searched a block's raw text for the `coco_managed` declaration, comments included. A user-owned edge type whose block said `// coco_managed: Bool? was deliberately omitted` was therefore classified as the connector's, and dropping the app took it along — the user's edge type and every edge in it, deleted on the strength of a comment. Reproduced live. `release_ownership` had the same blind spot and would have edited the comment. Both now work on the comment-blanked view the block scanner already uses, cutting matched declarations out of the original text by offset, so only a real declaration confers ownership and comments are left exactly as written.
system -> user -> system with the schema unchanged throughout: the first handoff dropped `coco_managed` from the block, but the second wrote nothing, because a system-managed declaration only wrote when the tracked schema differed. The block stayed without its ownership property, and dropping the connected app then failed with the edge type reported as not managed by this connector. Reproduced live. A system-managed declaration whose previous record is user-managed now schedules an upsert of its block even when nothing else changed, so reclaiming a type restores `coco_managed` the way releasing removed it. The ownership matrix test pinned the old "writes nothing" outcome for exactly these cases; its expectation is updated.
A `Date` key is legal in `.pg`, and mounting such a node type was
allowed, but declaring a node then failed: `declare_node` put the raw
`datetime.date` into the target-state key, which the engine refuses
("Unsupported StableKey Python type"). The previous round's test only
constructed the target and never declared through it.
The target-state key is now the key property's encoded value — the
ISO string for a `Date`/`DateTime`, unchanged for a string or integer
— which is also what the graph is sent and keys on, so the tracked
identity and the stored key cannot disagree. The custom-encoder
refusal on key properties stays, with its reason restated: edges
address a node by the raw key value its author passed, so a key the
graph encodes differently is one no edge could ever reach.
Omnigraph identifies a `DateTime`-keyed node by the instant, as epoch milliseconds: `12:00+00:00` and `14:00+02:00` are one node, precision below a millisecond is dropped, and a naive value is read as UTC (all verified against the engine). The connector tracked such a key by its ISO spelling, so those two declarations produced one graph node with two tracking keys. Removing one declaration deleted the node the other still declared, and the survivor's later unchanged updates left it missing. Reproduced live. The tracking key for a `DateTime` property is now the instant in epoch milliseconds, computed the way the engine does. Two spellings of one instant are one target state — declared together the engine refuses them as a duplicate, declared in turn they update one node — and a `Date` key stays tracked by its ISO form, which is already one-to-one.
The DateTime tracking-key normalisation only applied to `datetime`
instances. A hand-built `PropertyDef("at", "DateTime")` lets a dict row
carry an ISO string, and a string fell through unchanged — so
`12:00+00:00` and `14:00+02:00` as strings still got two tracking keys
for one graph node, removing one declaration deleted the node, and the
survivor's unchanged updates left it missing. Reproduced.
The instant can only be derived from a `datetime`, and parsing every
spelling the engine accepts would mean re-implementing its parser, so a
`DateTime` key now refuses anything but a `datetime.datetime`, and a
`Date` key anything but a `datetime.date` (a `datetime` is a `date`
subclass whose ISO form carries a time). Both fail at `declare_node`,
naming the property, before any state is declared.
A hand-built `PropertyDef("at", "DateTime")` carries no encoder, and
encoding was keyed on that field alone. So the recommended `datetime`
value reached `json.dumps` raw ("Object of type datetime is not JSON
serializable"), a `date` key reached the engine's StableKey raw, and
writing `datetime.isoformat` in by hand was refused as a custom key
encoder — while the same schema inferred from a dataclass worked only
because `from_class` filled the built-in encoder in.
The built-in encoding now follows the `.pg` type: a definition with no
encoder gets the ISO encoding for `Date`/`DateTime`, mapped over a list
of either, exactly as an inferred one does. The `isoformat` methods
themselves count as built-in on a key property, since they are the same
encoding. Positive end-to-end tests cover an explicit `DateTime`-keyed
and an explicit `Date`-keyed schema with dict rows.
datetime.date.isoformat was accepted on a DateTime key. Applied to a datetime it drops the time, so noon and 14:00 on one date were stored as a single midnight node while tracked as two instants, and removing one declaration deleted the node the other still declared. The permitted key encoders are now listed per type.
A changed encoder rewrites the value the graph holds, but the rendered .pg is identical, so the type never noticed. Reconciling each node re-encodes and catches it, yet a memoized declaring component is skipped whole and reconciles nothing: str.lower to str.upper left lowercase values stored after a successful update. The type's tracking record now carries an identity for every property's encoder (qualified name, plus a digest of the code for a Python function). A change is a lossy invalidation, which also invalidates the memo of every component that declared into the type, so it runs again and its rows are re-upserted.
Deleting a node cascades to its edges in the graph, but the component that declared those edges never hears of it: its tracking still says they exist, so when the node came back the edges were not re-inserted and the graph ended with both endpoints and no edge. A sync that deletes nodes now reads the schema, and for each node of a type some edge type has as an endpoint, reads that node's edges. A node an edge outside the batch still references is reduced to the key-only stub an edge arriving before its node leaves behind, so the edge survives and the node's next declaration fills it in. A node undeclared together with all of its edges is deleted as before. The rendered query type is used for reads now too, so it is Query.
Dropping a node type took every referencing edge type along whose block carried coco_managed, and that property only said some app of this connector wrote the block. With app A owning Source and app B adopting it as user-managed while owning Supports: Source -> Claim, dropping A silently deleted B's Supports type with every edge in it, and B's next update did not restore it. The ownership property now names the app: coco_managed_by_<app>, from AppConfig.name (sanitized to an identifier, with a digest when that changes it). A drop takes along only edge types marked as this app's and refuses, naming the owner, for one marked as another app's; a handoff releases only this app's own marker. The app is in the property's name because the engine refuses to retype a property but applies an add and a drop of nullable properties in one migration. The app is read from the component context, so node_target and edge_target must run inside a component; a small getter exposes the app name on ComponentProcessorContext for that.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Node and edge types with additive .pg schema evolution, synthetic coco_key addressing, scratch-branch commits for oversized syncs, and reactive endpoint stubs for edges that race ahead of their nodes. Ships with docs and a meeting-notes knowledge-graph example.
🚧 WIP
/сс @aaltshuler